Same feedback I had on the other issue. The engine assumes the shell is initialized synchronously everywhere. I don't support any change that relies on callbacks instead of std::future's to guarantee safety.

hi，I've tried to use promise/future before, but there are serious problesm in this case

• first , it may disturb the sequence ( shell create )
  • There are many thread switches in the creation sequence when create shell，if future.get() called(block thread) on platform/ui/gpu/io thead,may disturb the timing
• second,will bring burden to the user
  • android/ios must calledfuture.get() on newThread(can't be io/ui/gpu/platform)
• finally ,May be cause deadlock if user called future.get() on platformthread 。Because we need post a task to platformthread to excute shell.setup after engine created

//demo code on PlatformThread
auto future = Shell::Create(...);
future.get(); 

//shell.cc   (after review and modify)
//now on ui thread
auto engine_ref = std::make_unique<Engine>(... );
fml::TaskRunner::RunNowOrPostTask(
  shell->GetTaskRunners().GetPlatformTaskRunner(), fml::MakeCopyable([]() mutable{
      //dead lock  (setupmust called on platformthread)
      shell->Setup(...);       
 }));

• For safety, static method Create will guarantee shell full initialized , the callback will be called after all init sequence/setp excuted. So,In my understanding,the shell should be safe.

Looking forward to your reply

I appreciate your consideration and thought put into this =)

it may disturb the sequence

What you are referring to is the synchronization that the future guarantees. The alternative is a race condition that may crash sometimes. The worst kind of bug to track down.

will bring burden to the user

This is not really a burden.

May be cause deadlock if user called future.get() on platformthread

This is a risk but it doesn't go away if you use callbacks instead of futures. If you are in a position where you would get a deadlock with futures, you will have an uninitialized variable and will have to abort the operation somehow, or you erroneously went down a code path prematurely. You can mimic the checking of an unfinished future by using wait_for.

I think your aversion to using futures stems from unfamiliarity and not a solid technical basis. All the problems you have with futures exist with callbacks, they are just harder to verify and thus easier to convince yourself they are correct, more fragile, and harder to debug.

@liyuqian @lucky-chen I created some sample code that duplicates the deadlock predicament to play around with: https://gist.github.com/gaaclarke/84caf13aa0da0a7a2198247e2e6c8cd0

I think the easiest solution is to remove the dispatch to the platform thread. Just do those calculations up front on the platform thread or if the calculations depend on previous results create a partial result like this:

class ShellHolder {
  private:
    unique_ptr<Shell> _shell;
    std::future<PartialShell> _partial_shell;
  public:
    Shell* get() {
      if (_shell) {
         return _shell;
      }
      _shell = CompleteShell(_partial_shell.get());
      return _shell;
    }
};

@gaaclarke Thanks reply. Other suggestions are very good, but I still hold different views on callback/promise。

Suppose we have 2 method

• promise/future std::future<std::unique_ptr<Shell>> Create(...)
• callback void Create( callback)

when init on android /ios

use future

//on platformThread
void init(){
   auto future = create(...);

   //usage 1  
   future.get(); // bad code, The cost of blocking the main thread is very expensive on android/ios . 
                        //maybe disturb the sequence of createshell ,such as deadlock


   //usage2 check status
   if(isFutureReady(future)){
      //balaba
   }else{
    // called launchFlutter() on some point (such as start flutterActivity)
   }
}

void launchFlutter(){
    future.get();  // if ready, fine . else  block (bad)
    //balabala
}
bool isFutureReady(std::future& f){...}

use callback

but with callback , very simple and users have almost no burden

void init(){
   Shell::Create([]( std::unique_ptr<Shell> shell ){
        //on platformthread
        // no burder , no deadlock
        // will non't disturb the sequenc of createshell 
        shell_ = std:move(shell);
        //balabala
   })
}

As api user, only care about result output. Shouldn't to deal with some additional state. These states should be handled by api self.

in this case , Either return the shell(full init) or don't return。Return future is not a good way in this scene because the future carries intermediate states related to internal logic of api，

@gaaclarke @lucky-chen : I believe the problem of C++ std::future here is the lack of std::future::then. (We all get used to Future.then in Dart and I'm surprised that C++ doesn't have the counterpart.) The example above would be easy with std::future if the current experimental "then" can be used:

auto future = create(...);
future.then(callback);

Without then, I believe the best we can do now with future is std::async:

auto future = create(...);
std::async(std::launch::async, [...](){
  future.get();
  callback(...);
});

It ensures that the waiting happens on another thread (other than the platform thread) so it will not block or dead lock.

Note that the current callback implementation in this PR may also have problem: it's unclear what thread will execute the callback. Currently the UI thread executes it and avoids block and dead lock. But that violates calling WealkPtr in the same thread. If we correctly calls the Shell::Setup and subsequently the callback in the platform thread, then we end up with blocking and dead lock again.

@liyuqian @gaaclarke According to the suggestion of review, made some changes，may be better

new api is future<ShellHolder> createShellHolder(...)

usage 1 (sync mode):

auto shell_holder_future = createShellHolder();
auto shell_holder = shell_holder_future.get();
auto shell = shell_holder.makeShell();

usage 2 (async mode)

auto shell_holder_future = createShellHolder();
std::async(std::launch::async,[](){
    //wait engine create end
   auto shell_holder = shell_holder_future.get();
   //makeShell method guarantee called setUp on platformThread
   auto shell = shell_holder.makeShell();
  //...
});

for shellHolder.makeShell

  fml::AutoResetWaitableEvent latch;
  std::unique_ptr<Shell> shell_res;
  fml::TaskRunner::RunNowOrPostTask(PlatformTaskRunner,[]() {
        shell_->Setup()
        shell_res = std::move(this->shell_);
        latch.Signal();
      );
  latch.Wait();
return shell_res;

、 should be replaced with ,

Put TRACE_EVENT0 as the first line of the function and change "Shell::Create" to "Shell::CreateShellHolde"

Put TRACE_EVENT0("flutter", "Shell::Create"); as the first line of this function.

Nit: InitializeShellHolder or CreateShellHolder might be a better name than InitShellEnv as we usually don't encourage abbreviations like Env.

Remove // fml::AutoResetWaitableEvent latch;

Can std::promise<std::unique_ptr<Shell::ShellHolder>> shell_holder_promise; be put before line 340 so we can save line 343?

Document this function.

Document this function especially on how the new shell_holder_promise parameter should be given and used later.